fix: when delete resource if no CRD in the cluster #1044
Conversation
zdtsw
requested review from
VaishnaviHire and
ykaliuta
and removed request for
mattmahoneyrh
| @@ -359,6 +358,9 @@ func getResource(ctx context.Context, cli client.Client, obj *unstructured.Unstr | |||
| found.SetGroupVersionKind(obj.GroupVersionKind()) | |||
| err := cli.Get(ctx, types.NamespacedName{Name: obj.GetName(), Namespace: obj.GetNamespace()}, found) | |||
| if err != nil { | |||
| if strings.Contains(err.Error(), "no matches for kind") { | |||
| return nil, nil // ignore mising CRD error | |||
There was a problem hiding this comment.
Could it be ok to return new NotFound? Argumentation is the same as af749ec
There was a problem hiding this comment.
should we just handle the notfound directly in the getResource()?
and we
e.g
func getResource(ctx context.Context, cli client.Client, obj *unstructured.Unstructured) (*unstructured.Unstructured, error) {
found := &unstructured.Unstructured{}
// Setting gvk is required to do Get request
found.SetGroupVersionKind(obj.GroupVersionKind())
err := cli.Get(ctx, types.NamespacedName{Name: obj.GetName(), Namespace: obj.GetNamespace()}, found)
if err != nil {
if errors.Is(err, &meta.NoKindMatchError{}) {
return nil, nil // ignore mising CRD error
}
if client.IgnoreNotFound(err) == nil {
return nil, nil // not found resource
}
return nil, err
}
return found, nil
}
for the caller:
// Return if error getting resource in cluster
found, err := getResource(ctx, cli, obj);
if err != nil {
return err
}
There was a problem hiding this comment.
This will work, but I do not like the idea to return nil object with nil error. It's prone to mistakes. But I do not have a strong opinion. Let's ask one more point of view? :) @bartoszmajsak
There was a problem hiding this comment.
@zdtsw @ykaliuta In my opinion, returning nil, nil can cause more harm than good. The response should either be the requested item or an error. This way, you only need to check for nil in the case of an error, allowing you to handle exceptions appropriately. Otherwise, we risk cluttering the codebase with nil checks or encountering nil pointer errors at runtime, as the caller may assume the returned instance is valid if the error is nil. Therefore, I believe it is preferable to return apierrs.NewNotFound as @ykaliuta suggests.
There was a problem hiding this comment.
follow up in #1045 to address comments.
Signed-off-by: Wen Zhou <[email protected]>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ykaliuta The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-merge-bot
bot
merged commit c040f77
into
opendatahub-io:incubation
Signed-off-by: Wen Zhou <[email protected]> (cherry picked from commit c040f77)
- since we already have these resource for create, list, delete etc, no harm to add get on them Signed-off-by: Wen Zhou <[email protected]> (cherry picked from commit bd9b83c) fix: when delete resource if no CRD in the cluster (opendatahub-io#1044) Signed-off-by: Wen Zhou <[email protected]> (cherry picked from commit c040f77) deploy: refactor getResource to return NotFound for both cases (opendatahub-io#1046) Avoid returning (nil, nil), convert error if CRD not present to NotFound and handle them in the caller code. Avoid checking for found == nil but handle only error code. This is more idiomatic and less error prone. Signed-off-by: Yauheni Kaliuta <[email protected]> (cherry picked from commit 9651b76) Handle Kserve exception Fix csv
- since we already have these resource for create, list, delete etc, no harm to add get on them Signed-off-by: Wen Zhou <[email protected]> (cherry picked from commit bd9b83c) fix: when delete resource if no CRD in the cluster (opendatahub-io#1044) Signed-off-by: Wen Zhou <[email protected]> (cherry picked from commit c040f77) deploy: refactor getResource to return NotFound for both cases (opendatahub-io#1046) Avoid returning (nil, nil), convert error if CRD not present to NotFound and handle them in the caller code. Avoid checking for found == nil but handle only error code. This is more idiomatic and less error prone. Signed-off-by: Yauheni Kaliuta <[email protected]> (cherry picked from commit 9651b76) Handle Kserve exception Fix csv (cherry picked from commit e5f65bd)
Description
ref: https://issues.redhat.com/browse/RHOAIENG-8297
How Has This Been Tested?
Merge criteria: